Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scripting for custom property types #3971

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

dogboydog
Copy link
Contributor

@dogboydog dogboydog commented Jun 16, 2024

Aims to fix #3419

Related to #2902

Demo of the functionality so far in the console:

> for (let t of tiled.project.propertyTypes) { tiled.log( t.name) }
Breakable
Door
Enemy
KillZone
> tiled.project.findTypeByName('Enemy').name
$0 = Enemy
> tiled.project.findTypeByName('Enemy').color
$1 = #ff0000
> tiled.project.findTypeByName('Enemy').isClass
$2 = true 
> tiled.project.findTypeByName('Enemy').usageFlags  & ClassPropertyType.LayerClass
$2 = 0
> tiled.project.findTypeByName('Enemy').usageFlags  & ClassPropertyType.MapObjectClass
$3 = 4

TODO:

  • Editing properties on custom types
  • Adding new enum and class types to the project
  • Add validation to prevent duplicate named types from being added to the project
  • Adding new members to class types, including members that are classes
  • Removing members from a class type
  • Removing types from the project
  • Add scripting doc (@dogboydog will do this but once the API contract is finalized)

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a great start, @dogboydog! There are some open questions, especially what to do with the raw pointer and potentially whether we should rather return class members as an array (though that does seem a little clumsy and somewhat pointless since internally it's still a map for now).


QColor color() const { return mClassType->color; }
// TODO: " No viable overloaded '=' "
// void setColor(QColor &value) { mClassType->color = value; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is just cause of passing by reference. The argument should either be by value or by const-reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of those compile right now.

void setColor(QColor value) { mClassType->color = value; }

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h:116: error: C2678: binary '=': no operator found which takes a left-hand operand of type 'const QColor' (or there is no acceptable conversion)

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h(116): note: Conversion loses qualifiers

Or
void setColor(const QColor &value) { mClassType->color = value; }

C:\Users\chris\tiled\src\tiled\scriptpropertytype.h:116: error: C2678: binary '=': no operator found which takes a left-hand operand of type 'const QColor' (or there is no acceptable conversion)
C:\Users\chris\tiled\src\tiled\scriptpropertytype.h(116): note: Conversion loses qualifiers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's just because mClassType is a const ClassPropertyType*, so you can't modify it. You'd need a non-const ClassPropertyType*.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have QSharedPointer<ClassPropertyType> mClassType, we can assign a new color to mClassType->color. Just note that a UI update will need to be triggered as well and we also might want to save the project. These things currently happen only from PropertyTypesEditor::applyPropertyTypes, which is called from all the places where changes are made to the types.

I'm not sure about triggering saving of the project immediately though. It would be better to schedule this to avoid too much file writing in case of changing a bunch of things from a script.

I think we could start with just triggering emit Preferences::instance()->propertyTypesChanged() after applying changes.

@dogboydog dogboydog force-pushed the 3419_scripting_custom_types branch from b1486e2 to e80e00b Compare September 7, 2024 00:47
@dogboydog
Copy link
Contributor Author

I rebased this from master and tried to take another look, but I still don't feel confident in how to proceed. PropertyTypes returns const pointers which I can't use to modify values nor to try to store a pointer back to the script type to manage cleaning up. Also, you mentioned that there may be a change in how property types are stored coming up, so that they aren't sorted by name and rather allow the user to re-order them, so if I were to try to change PropertyTypes now I'm not sure if that would conflict.

Sorry, I don't write much C++ and so I'm not used to dealing with pointers/const qualifiers

@bjorn
Copy link
Member

bjorn commented Sep 10, 2024

@dogboydog That's alright, thank you for rebasing the PR!

I would say not to worry about introducing conflicts, since I'd anyway be around to help resolve them again. But yeah, I understand the ownership and const stuff can be a little much if you're not used to C++. I'll see if I can have another look this week.

@dogboydog dogboydog force-pushed the 3419_scripting_custom_types branch 2 times, most recently from 5729b4e to 0232264 Compare January 17, 2025 16:55
bool drawFill() const { return mClassType->drawFill; }
// void setDrawFill(bool value) { mClassType->drawFill = value; }
int usageFlags() const { return mClassType->usageFlags; }
//void setUsageFlags(int value) { mClassType->setUsageFlags(value); }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setter for usage flags accepts bool value to either turn the specified flags on or off, but it doesn't in itself provide a way to just set the usage flags to the specified value. Would script writers expect to be able to just hard set the usage flags to a value (personally, this is what I think makes sense so that you can do things like &= , |= the flags) or have a method like setUsageFlags(flags, value: bool) ?

Without changing the setUsageFlags in propertytype.cpp, to achieve a setter, I could turn off all flags with setUsageFlags( ClassUsageFlag. AnyUsage , false) and then turn the specified flags on with setUsageFlags(flagsFromScriptUser, true) if that makes sense

Copy link
Member

@bjorn bjorn Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed that signature is quite UI-oriented since it's used in response to the user toggling checkboxes. I agree from a scripting API point of view, we probably just want to be able to set the flags directly and you could introduce a void ClassPropertyType::setUsageFlags(int flags) overload for this (though actually two calls, like you suggested, is also a fine solution).

@dogboydog dogboydog force-pushed the 3419_scripting_custom_types branch from d293bea to 4a697eb Compare January 24, 2025 01:54
@dogboydog dogboydog force-pushed the 3419_scripting_custom_types branch from b3f333e to f02a78f Compare January 28, 2025 22:40
@bjorn bjorn force-pushed the 3419_scripting_custom_types branch from f02a78f to da70b69 Compare March 6, 2025 13:03
@dogboydog
Copy link
Contributor Author

dogboydog commented Mar 6, 2025

Name duplication validation seems to be working
image

Added the ability to remove a class's member

Maybe putting the script error utility for duplicate names in project.h/project.cpp isn't that clean since the Project class isn't really related to scripting. But I wasn't sure how to get the current EditableProject instead

bjorn added 5 commits March 7, 2025 17:39
Like this it could be somewhat useful, since now the value is typed
correctly.
With two overloads, one for raw integer values and one for PropertyValue,
for convenience. The latter raises an error if it's not of the right
type.
static_cast can be used to access the subclasses instead of storing
more specifically typed shared pointers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripting: the API for getting PropertyTypes
2 participants